-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to v2 of coffee #172
base: master
Are you sure you want to change the base?
Upgrade to v2 of coffee #172
Conversation
…ded classes, __super__ issue, Interpolated strings issue.
Thanks for getting this started @brucejo75! Do you intend to keep going with the three areas listed above? I'll be happy to merge and push a release once everything looks ready. |
Thanks @vsiverson! Yeah, I did those fixes in the pr and it all works now for my project in v1.6.1. I am trying to find the errors in the tests. I fixed one. Working on the Slow going, I am Coffeescript illiterate and BTW: what do you think about changing the version to 2.0.0? I thought it was the only way to go because CS 1 & 2 are incompatible. |
Hey @vsiverson, can you point me to where the I am just dropping on Or better yet, how can I run a debugger with |
@brucejo75 Hey, thanks for working on this today. Unfortunately I'm busy for the rest of the day, I'll try to take a look at what you've done over the weekend and try to answer your questions above. I'm pretty rusty on this code myself at this point. Reminder to self, I should probably look into updating resumable.js as well in this release. Resumable is definitely a messy codebase, and there is a fork that seems to be better maintained (name is escaping me), but I'm probably not going to put in the work to manage a probably breaking change like that. |
Hi @vsiverson, can you point me to where to look? I will be happy to run this down. Thanks! |
Okay, I took a quick look. The tests only seem to be failing on the server-side which is curious, the same tests run from the client are all working. It's not immediately clear to me what would cause that... Note that resumable.js is not actually being used in any of these failing tests. These are tests of the backend API that resumable.js uses, but the tests themselves are simple and mock the library. So there's no need to be digging around in resumable.js itself. The resumable.js mock is: The failing tests start here (again, failing on the server-side only): Not sure if that helps you out much or not. I really haven't had time to come up to speed on the differences in CoffeeScript2 because my motivation to do so is low, given that I do not currently plan to use Coffeescript (1 or 2) in any future projects. |
I just tried running off your branch and I am seeing the same errors on the server-side when I run |
Just looking quickly: https://github.com/vsivsi/meteor-file-collection/blob/master/test/file_collection_tests.coffee#L465-L524 has some string interpolations of |
Thanks @vsiverson for your response. Thanks @edemaine for identifying the issues! @vsiverson, all tests now pass. Ready to roll! |
Great news! Okay, so I'm super annoyed that for any version after Meteor 1.6, the Some other TODOs before release...
Also, @brucejo75 mentioned a major version bump to 2.0... Something like that is probably necessary. But here's an idea to ponder.... Some time ago I created a new Github Org called "Darkmattergy" (like Dark Matter + Energy, in keeping with the Meteor cosmic theme). I'm considering transferring file-collection and job-collection to that org on github, and setting up matching orgs on Atmosphere/NPM so that these projects and their associated bits can live on without my direct involvement. The "vsivsi" versions will deprecate with their Meteor 1.5.x support intact, and for Meteor 1.6+, everything will switch over to the new Org. The job-collection package is a lot more popular than this one, and a similar effort to port it over with minimal involvement from me has happened there, so these are both pretty close to being ready to release. Anyway, this seems like the best moment to make such a move if it is ever going to happen. We have breaking changes in both these packages and the Meteor + Coffeescript platform. There's a clear boundary here between what can be deprecated and what can be supported moving forward etc. What do you guys think? Are you willing to be founding members of this new org (with the privileges and responsibilities that may entail?) |
BTW, here are all of the latest versions of the dependent packages...
|
It's been so long, I somehow forgot that when running tests in a package directory you need to specify the CWD to the command: |
To be honest, my plan is to switch over to using I adopted For my application My goal with this PR was simply to get it running with Meteor 1.6.1. Also, I know nothing about coffeescript. I do not understand the syntax and I think the whole concept is rife with issues (like adding additional dependency complexity that necessitated this PR). I think your plan is a good one and the right thing to do. And I would be happy to help a little bit to get over the hump. But for my purposes it simply an exercise in doing the simplest thing for my app (make it work with Meteor v1.6.1 and not rewrite for |
Sad to see people leaving both CoffeeScript and Meteor! I am still an active user of both, and of this package. I'm willing to help (some) with this package (e.g. I still want to add proper mobile support, just haven't gotten to it yet), but less so with the entire organization. |
I haven’t stopped liking CoffeeScript, it was a great tool for its time. But in the ES6+ world, it’s just less necessary, and with the rise of more complex JS build toolchains it just adds one more source of potential issues and maintenance (this PR being exemplary!) Plus TypeScript has a lot of merits for larger projects, but those merits are incompatible with the remaining benefits of CoffeeScript. I’m also not down on Meteor per se, my work just no longer requires it. I don’t think it has in any way out-lived it’s usefulness. If anything, I think it has only recently stabilized to the point where I’d feel comfortable building something “real” with it. I do fear what happens if/when the MDG transitions to the next thing though. Meteor has a lot of moving parts, and that complexity will be difficult to maintain on a volunteer basis. Anyway, me pulling away from this is about my situation, not a mark against these techs. |
@vsivsi @brucejo75 Is there a chance this PR will be merged soon? Thanks for all the work! |
src/gridFS_server.coffee
Outdated
@@ -258,7 +265,7 @@ if Meteor.isServer | |||
writeStream.on 'expires-soon', () => | |||
writeStream.renewLock (e, d) -> | |||
if e or not d | |||
console.warn "Automatic Write Lock Renewal Failed: #{file._id}", e | |||
console.warn "Automatic Write Lock Renewal Failed: #{file._id.str}", e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended? I believe it should be _id._str
instead of _id.str
Sorry to jump in, but what is lacking for this to be merged? I can't wait to use this project in Meteor 1.6, please tell us |
thanks @antoninadert, I just pushed a fix for that one. |
Any chance of merging this soon? |
Also looking forward to see this merged! |
When merge? |
I think we need a new README to explain which version works with which version of Meteor and then merge. @brucejo75 What do you think? |
FWIW, I just published |
Thanks @edemaine, Do you have any interest in taking over this project for @vsiverson, he has a list of todos before he was willing to take the PR. And he is looking for someone to own this project and I am not the right person. |
@brucejo75 @vsiverson Yes, I'm willing to help maintain file-collection. It sounds like this PR is done, just needs another review; and then the other to-do items could be turned into issues and addressed by future PRs before release. |
@edemaine @brucejo75 Any interest in maintaining this package if it is transitioned to the Meteor Community Packages organization? See: https://github.com/Meteor-Community-Packages My other main meteor package, job-collection, looks to be moved over there, and I'd like to do the same for this one, assuming people are still using it, as I'm no longer an active Meteor developer. See these discussions for more info: |
@vsiverson Yep. I can try to review PRs at least, and I do have some old plans to add mobile support eventually (which we discussed long ago). Should we open a new issue on https://github.com/Meteor-Community-Packages/organization/issues/? |
I think so. I'm not sure what their workflow is over there. You might ask @mitar for guidance if it's not clear, as he seems to be spearheading the effort for job-collection. Given the shared history of these two packages, it might be smoother to try to do them together. |
Yes, feel free to transfer packages to me and I will move them over to common organization. Because they are hosted currently on personal GitHub account I think this is the easiest. |
(Also add |
Make meteor-file-collection work with V2 of Coffescript (specifically 2.0.3.3).
Changes to
package.js
[email protected]_3
.vsivsi:meteor-file-collection
to2.0.0
. Because of the dependency on[email protected]_3
.I believe all changes are backward compatible with v1 of CoffeeScript. But I did not retest them.
3 areas needed to be addressed:
super
andthis
Needed to refactor the FileCollection class constructor for both client and server.
FileCollection.__super__
does not existSimply set
FileCollection.__super__
toMongo.Collection.prototype
String interpolations behave differently
In v1 a string interpolation for an
ObjectID
would resolve to the string portion of the object e.g. the_str
property. This is no longer the case with v2.